feat(search-indexer): Move mapping logic to separate class #16410
Conversation
WalkthroughThe changes in this pull request introduce a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
libs/cms/src/lib/search/mapping.service.ts (1)
87-92: Specify return type formapDatamethodFor better readability and maintainability, explicitly declare the return type of the
mapDatamethod. This helps with type checking and understanding the method's output.Apply this diff to specify the return type:
-mapData(entries: processSyncDataInput<unknown>) { +mapData(entries: processSyncDataInput<unknown>): MappedDataType[] { // Replace MappedDataType with the appropriate type
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/cms/src/lib/search/cmsSync.module.ts (2 hunks)
- libs/cms/src/lib/search/cmsSync.service.ts (3 hunks)
- libs/cms/src/lib/search/contentful.service.ts (5 hunks)
- libs/cms/src/lib/search/mapping.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/cms/src/lib/search/cmsSync.module.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/search/cmsSync.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/search/contentful.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/search/mapping.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (8)
libs/cms/src/lib/search/cmsSync.module.ts (1)
35-35: LGTM! MappingService successfully integrated.The changes look good. The MappingService is correctly imported and added to the providers array, making it available for dependency injection within the module. This aligns well with the PR objectives of introducing a new MappingService class to enhance the mapping process of CMS data.
Remember to update the documentation to reflect the addition of this new service and its role in the CMS synchronization process.
Let's verify if the new MappingService affects the module's exports:
This will help us understand if the MappingService needs to be exported from this module for use in other parts of the library.
Also applies to: 73-73
✅ Verification successful
Let's rerun the verification with the corrected file type.
: MappingService is correctly integrated within the
cmsSync.module.ts. It is properly imported and provided, ensuring seamless dependency injection for all internal services. No changes to module exports are necessary as MappingService is used exclusively within the library.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if MappingService is used in other files within the library # Search for MappingService usage rg --type typescript "MappingService" libs/cms/src/lib/searchLength of output: 96
Script:
#!/bin/bash # Description: Check if MappingService is used in other files within the library # Search for MappingService usage in TypeScript files rg --type ts "MappingService" libs/cms/src/lib/searchLength of output: 667
libs/cms/src/lib/search/mapping.service.ts (1)
1-28: Imports are well-organized and necessary dependencies are appropriately includedAll imported services are correctly specified and align with the dependencies used in the class. There are no unused imports.
libs/cms/src/lib/search/cmsSync.service.ts (1)
15-15: Refactoring enhances modularity and reusabilityThe introduction of
MappingServiceand its integration intoCmsSyncServiceeffectively separate the mapping logic from the sync service. This improves modularity and aligns with the coding guidelines by promoting reusability of components across different NextJS apps. It also leverages TypeScript's strengths in defining types and interfaces.Also applies to: 38-40, 157-157
libs/cms/src/lib/search/contentful.service.ts (5)
27-27: Verify Import Path forMappingServiceEnsure that the import path
'./mapping.service'correctly points to theMappingServicemodule and that it is properly exported.
83-83: Dependency Injection ofMappingServiceThe
MappingServiceis being injected into the constructor and assigned to a private readonly property. This aligns with best practices for dependency injection.
588-593: Proper Mapping Before Bulk IndexingThe data fetched from Contentful is mapped using
this.mappingService.mapData(items)before being indexed into Elasticsearch. This ensures that the data conforms to the expected structure.
604-607: Ensure Accurate Nested Entries Progress LoggingThe logging statement uses
nestedProgressandtotalNestedto report progress. Verify thatnestedProgressis being updated correctly within the loop to reflect the actual number of nested entries processed.
673-673: Confirm Modification ofindexableEntriesWhen calling
this.resolveNestedEntries, ensure that any entries added or modified within this method are correctly reflected inindexableEntries, especially if they need to be processed later.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16410 +/- ##
=======================================
Coverage 36.73% 36.73%
=======================================
Files 6808 6809 +1
Lines 141069 141087 +18
Branches 40219 40227 +8
=======================================
+ Hits 51818 51825 +7
- Misses 89251 89262 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 9 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
kodiakhq
bot
deleted the
feature/search-indexer-seperate-mapping-logic
branch
Move mapping logic to separate class
What
Now there is a MappingService class that takes care of mapping cms data into elasticsearch data.
Instead of writing all resolved root entries from nested entries alongside other entries we now separately write them down to elasticsearch to reduce the memory load
Checklist:
Summary by CodeRabbit
New Features
MappingServiceto enhance content synchronization and mapping capabilities.ContentfulServiceto improve nested entries resolution with progress tracking.Bug Fixes
CmsSyncServiceto streamline dependency management and improve data processing.Improvements
doSyncmethod inCmsSyncServicefor a more centralized mapping approach.resolveNestedEntriesmethod inContentfulServicefor better handling of indexable entries.